-
-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Priority fees on EVMs (EIP-1559) #16342
base: develop
Are you sure you want to change the base?
Conversation
🚀 Expo preview is ready!
|
1976ef5
to
51b93d9
Compare
8b615ab
to
a02ee59
Compare
a02ee59
to
bef5516
Compare
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
9062eec
to
816379a
Compare
edf2a0c
to
a0c530d
Compare
I separated misc levels from bitcoin, can you please check? @marekrjpolak In the future it would make sense to separate Ethereum from misc too, I think. And then Solana when there will be priority fees as well. But for now I think this is sufficient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few cosmetic nits from me
@@ -187,6 +188,7 @@ const EthereumDetails = ({ | |||
networkType, | |||
}: DetailsProps) => { | |||
const hasSettlementLayer = !!getNetwork(symbol).settlementLayer; | |||
const locale = useLocales(); | |||
|
|||
// TODO: this can be probably moved to FeeRate component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be moved according to the comment? and also getFeeRate in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
if (network.networkType === 'ethereum') { | ||
// NOTE: ethereum smart fees are not implemented properly in @trezor/connect Issue: https://github.com/trezor/trezor-suite/issues/5340 | ||
// create raw call to @trezor/blockchain-link, receive data and create FeeLevel.normal from it | ||
const feeLevels: 'preloaded' | 'smart' = getNetworkFeatures(network.symbol).includes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
const feeLevels: 'preloaded' | 'smart' = getNetworkFeatures(network.symbol).includes( | |
const feeLevels = getNetworkFeatures(network.symbol).includes( |
maxFeePerGas: precomposedTransaction.maxFeePerGas ?? undefined, | ||
maxPriorityFeePerGas: precomposedTransaction.maxPriorityFeePerGas ?? undefined, | ||
gasPrice: | ||
!precomposedTransaction.maxFeePerGas && precomposedTransaction.feePerByte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Imo moving this ternary to separate const before const transaction = prepareEthereumTransaction({
would make it more readable
// validate if token balance is not 0 or lower than amount | ||
if ( | ||
availableTokenBalance && | ||
(availableTokenBalance === '0' || new BigNumber(amount).gt(availableTokenBalance)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if amount < 0 ?
feePerByte: feeLevel.feePerUnit, | ||
feeLimit: feeLevel.feeLimit, | ||
token, | ||
bytes: 0, // TODO: calculate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not to calculate in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the code and found no major problems. My comments are rather code-style/readability proposals.
Although it is quite a big change, so make sure that QA will test both ETH priority fees and All the other network fees (even in mobile app) thoroughly before merging this 🙏
...e/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx
Outdated
Show resolved
Hide resolved
@@ -17,6 +17,11 @@ export interface FormState { | |||
setMaxOutputId?: number; | |||
selectedFee?: FeeLevel['label']; | |||
feePerUnit: string; // bitcoin/ethereum/ripple custom fee field (satB/gasPrice/drops) | |||
maxPriorityFeePerGas?: string; // ethereum eip1559 only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about moving all these new attributes to eip1559
object? That would make it more readable, especially when this type is shared for all the networks.
So e.g. FormState.maxPriorityFeePerGas
would be --> FormState.eip1559.axPriorityFeePerGas
etc.
What do you think?
async load(blockchain: Blockchain) { | ||
if (this.coinInfo.type !== 'bitcoin') return this.loadMisc(blockchain); | ||
if (this.coinInfo.type !== 'bitcoin') return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change the type of coinInfo
received in constructor to BitcoinNetworkInfo
, then we can remove this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, the BitcoinFees
/MiscFees
separation looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will actually create a separate PR only for this part, this PR is already too big and it will be safer to merge it in parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be pretty uncontroversial PR.
3b7cc44
to
ecf33d0
Compare
ecf33d0
to
aad7d97
Compare
Description
Added priority fee for all EVM chains except BSC and Ethereum classic (not supported)
Bsc has base fee always at 0, so it's needed to adjust the logic for bsc separately.
Refactored Fee component
Connect part of this PR was moved to #17156
Related Issue
Resolve #16438
Screenshots: